Win32: Spawn extension processes in job #7838
Conversation
This puts any processes spawned by the extension in a job, so that they can be terminated when the main application exits. Signed-off-by: Mark Yen <mark.yen@suse.com>
When we fail to open a process (which we'll do a lot because we shouldn't be able to affect processes under the system account), only show a message in debug logging instead of all the time. Signed-off-by: Mark Yen <mark.yen@suse.com>
| } | ||
| type JOBOBJECT_EXTENDED_LIMIT_INFORMATION struct { | ||
| BasicLimitInformation JOBOBJECT_BASIC_LIMIT_INFORMATION | ||
| IoInfo struct { |
There was a problem hiding this comment.
nit; can you put the IoInfo outside as you did with JOBOBJECT_BASIC_LIMIT_INFORMATION so it's easier on the eyes when looking at the reference?
| heapFree = hKernel32.NewProc("HeapFree") | ||
| ) | ||
|
|
||
| // Convert a list of arguments into a command line for use with CreateProcess. |
There was a problem hiding this comment.
| // Convert a list of arguments into a command line for use with CreateProcess. | |
| // buildCommandLine converts a slice of arguments into a properly formatted command line string | |
| // suitable for use with the CreateProcess API. This function is the reverse of | |
| // `windows.DecomposeCommandLine()`, which parses a command line string into individual arguments. | |
| // | |
| // The function follows the parsing rules for command-line arguments as outlined in: | |
| // https://learn.microsoft.com/en-us/cpp/c-language/parsing-c-command-line-arguments | |
| // | |
| // Key behaviors include: | |
| // - The first argument (typically the executable name) is treated specially and enclosed in double quotes | |
| // without applying backslash escape rules, including for embedded quotes. | |
| // - Each subsequent argument is wrapped in double quotes, and any internal quotes or backslashes are | |
| // escaped appropriately according to the rules for Windows command-line parsing: | |
| // - Backslashes preceding a quote are doubled (e.g., `\\` becomes `\\\\`), and the quote itself is escaped. | |
| // - Backslashes followed by non-quote characters are preserved as-is. | |
| // - The function ensures that the final command line is properly formatted for passing to `CreateProcess`, | |
| // handling edge cases such as arguments containing spaces, quotes, or backslashes. |
There was a problem hiding this comment.
Mostly taken, but with some tweaks (and checking with godoc to ensure the HTML rendering is correct).
| func buildCommandLine(args []string) string { | ||
| // See https://learn.microsoft.com/en-us/cpp/c-language/parsing-c-command-line-arguments | ||
| // for details on how it must be parsed. | ||
| var result []byte |
There was a problem hiding this comment.
I think for better performance when dealing with string concatenation (especially when appending repeatedly in a loop) it makes sense to use string builder instead since it is designed for string concatenation operations.
Also, you are calling append so many times in a loop which could be inefficient.
There was a problem hiding this comment.
Looking at strings.Builder, that's actually even more awkward (because the WriteString API says it must always return a nil error which we must then check (because errcheck). I guess it's slightly more efficient in that the buffer it allocates is not zeroed, and it grabs the data directly when returning the string? I'll use it then.
| // We need the handle to have a stable address for the jobs list; we | ||
| // do this by allocating memory in C to avoid the golang GC moving | ||
| // things around. | ||
| heap, _, err := getProcessHeap.Call(0, 0, 0) |
There was a problem hiding this comment.
I think it should be fine to call getProcessHeap.Call() without any args.
There was a problem hiding this comment.
It is; I have no idea why I decided 0, 0, 0 was useful.
|
|
||
| jobList, _, err := heapAlloc.Call(heap, 0, unsafe.Sizeof(job)) | ||
| if jobList == 0 { | ||
| return nil, fmt.Errorf("failed to allocate memory: %s", err) |
There was a problem hiding this comment.
| return nil, fmt.Errorf("failed to allocate memory: %s", err) | |
| return nil, fmt.Errorf("failed to allocate memory: %w", err) |
There was a problem hiding this comment.
Thanks; also couple attribute list errors below.
| defer func() { | ||
| _ = windows.CloseHandle(job) | ||
| }() | ||
| if !errors.Is(err, os.ErrExist) { |
There was a problem hiding this comment.
| if !errors.Is(err, os.ErrExist) { | |
| // Check if the job was newly created or already exists | |
| if !errors.Is(err, os.ErrExist) { |
|
|
||
| // Set the job so processes can't break away, and it terminates when the | ||
| // last handle is closed. | ||
| var limits JOBOBJECT_EXTENDED_LIMIT_INFORMATION |
There was a problem hiding this comment.
Code organization suggestion:
// configureJobLimits sets the job limits to prevent breakaway processes and ensures
// the job terminates when the last handle is closed.
func configureJobLimits(job windows.Handle) error {
var limits JOBOBJECT_EXTENDED_LIMIT_INFORMATION
ok, _, err := queryInformationJobObject.Call(
uintptr(job),
JobObjectExtendedLimitInformation,
uintptr(unsafe.Pointer(&limits)),
unsafe.Sizeof(limits),
uintptr(unsafe.Pointer(nil)))
if ok == 0 {
return fmt.Errorf("error looking up job limits: %w", err)
}
// Modify job limits to prevent breakaway and ensure termination on job close
limits.BasicLimitInformation.LimitFlags &= ^(JOB_OBJECT_LIMIT_BREAKAWAY_OK | JOB_OBJECT_LIMIT_SILENT_BREAKAWAY_OK)
limits.BasicLimitInformation.LimitFlags |= JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE
ok, _, err = setInformationJobObject.Call(
uintptr(job),
JobObjectExtendedLimitInformation,
uintptr(unsafe.Pointer(&limits)),
unsafe.Sizeof(limits))
if ok == 0 {
return fmt.Errorf("error setting job limits: %w", err)
}
return nil
}
There was a problem hiding this comment.
Add a helper function? Sure, I'll do that.
|
|
||
| // Duplicate the job into the given process (but leaking it). This way when | ||
| // the target process exits, it will shut down the job. | ||
| hProc, err := windows.OpenProcess(windows.PROCESS_DUP_HANDLE, false, pid) |
There was a problem hiding this comment.
Also,
// injectJobHandleIntoParent duplicates the job handle into the given parent process
// so the parent can manage the job's lifecycle.
func injectJobHandleIntoParent(pid uint32, job windows.Handle) error {
hProc, err := windows.OpenProcess(windows.PROCESS_DUP_HANDLE, false, pid)
if err != nil {
return fmt.Errorf("failed to open parent process %d: %w", pid, err)
}
defer windows.CloseHandle(hProc)
err = windows.DuplicateHandle(windows.CurrentProcess(), job, hProc, nil, 0, false, 0)
if err != nil {
return fmt.Errorf("failed to duplicate job handle: %w", err)
}
return nil
}
| t.Parallel() | ||
| cases := [][]string{ | ||
| {"arg0", "a b c", "d", "e"}, | ||
| {"C:\\Program Files\\arg0\\\\", "ab\"c", "\\", "d"}, |
There was a problem hiding this comment.
Should there also be a test for mixed forward and backward slashes or is that too crazy?
e.g C:/Program Files\\Test
There was a problem hiding this comment.
Added test. This also happened to catch an error when there are no arguments :)
|
|
||
| func TestBuildCommandLine(t *testing.T) { | ||
| t.Parallel() | ||
| cases := [][]string{ |
There was a problem hiding this comment.
There are some more cases that I can think of, but I will leave it up to you to see if they make sense or not:
- Spaces in the middle of the argument
- Trailing spaces (extra spaces after quotes, etc)
- Windows-specific characters that are valid in the command line and have special meaning e.g.
&, |, >, <, etc.
There was a problem hiding this comment.
Thanks, I'll add those. (The ones here are mostly from the documentation.) Though the first one is covered with de fg.
Signed-off-by: Mark Yen <mark.yen@suse.com>
This puts any processes spawned by the extension in a job, so that they can be terminated when the main application exits.
We intentionally let the main Rancher Desktop process have a handle to the new job (which isn't ever referenced), and set the job up so that it will automatically terminate when all handles to that job are closed. This means that as the main process exits, it will automatically cause all processes in that job to terminate (because it would have the only outstanding handle to the job).
For #7653